Skip to content

fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165

Open
brandyscarney wants to merge 27 commits into
nextfrom
FW-6922
Open

fix(overlays): show focus indicators only during keyboard navigation and improve focus management#31165
brandyscarney wants to merge 27 commits into
nextfrom
FW-6922

Conversation

@brandyscarney

@brandyscarney brandyscarney commented May 22, 2026

Copy link
Copy Markdown
Member

Issue number: internal


What is the current behavior?

The focus indicator is displayed on the option when opening certain interfaces by clicking on the Select component:

Modal Popover
select-modal select-popover

What is the new behavior?

  • Hide the focus indicator when the Select component is opened using pointer events
  • Display the focus indicator when the Select component is opened using keyboard navigation (Space or Enter)
  • Cycle focus through the Select elements (radios, handle, cancel buttons) in the correct order and allow Shift + Tab to go backwards in order
  • Removes the native focus from Checkbox and does not allow focusable when inside of an item (this matches Radio's behavior)
  • Adds e2e tests for focus functionality in both Select and Modal to avoid future regressions

Does this introduce a breaking change?

  • Yes
  • No

Other information

@brandyscarney brandyscarney requested a review from a team as a code owner May 22, 2026 16:22
@brandyscarney brandyscarney requested a review from thetaPC May 22, 2026 16:22
@brandyscarney brandyscarney marked this pull request as draft May 22, 2026 16:22
@vercel

vercel Bot commented May 22, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
ionic-framework Ready Ready Preview, Comment Jun 16, 2026 6:49pm

Request Review

@@ -114,10 +114,6 @@ slot[name="end"]::slotted(*) {

// Item in Select Modal
// --------------------------------------------------
:host(.in-select-modal) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to hide the focused state.

@@ -15,11 +15,6 @@ ion-item {
--border-width: 0;
}

ion-item.ion-focused::part(native)::after {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added to hide the focused state.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ionic theme wasn't added to this test so now it is generating screenshots.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focus indicator is no longer displayed when opening with a mouse click.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The focus indicator is no longer displayed when opening with a mouse click.

@@ -8,7 +8,7 @@ import { configs, test } from '@utils/test/playwright';
* does not. The overlay rendering is already tested in the respective
* test files.
*/
configs({ directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {
configs({ modes: ['ios', 'md', 'ionic-md'], directions: ['ltr'] }).forEach(({ title, config, screenshot }) => {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now checks for the ionic theme and captures screenshots to avoid future focus behavior regressions.

@brandyscarney brandyscarney changed the title fix(overlays): properly focus elements in a sheet modal fix(overlays): show focus indicators only during keyboard navigation and improve focus management Jun 2, 2026
@brandyscarney brandyscarney marked this pull request as ready for review June 2, 2026 18:27

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking really good! I tested the functionality and the changes here seem to work fine, but I noticed some things and just wanted a bit of clarification. Good work so far, though!

Comment thread core/src/utils/overlays.ts Outdated
Comment thread core/src/components/checkbox/checkbox.tsx
Comment thread core/src/components/select/test/basic/select.e2e.ts

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great to me! Thanks for fixing that issue and addressing the questions I left before! Just a very minor non-blocking question

Comment thread core/src/components/modal/test/sheet/modal.e2e.ts Outdated

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Questions:

  1. Multiple-select interfaces: if a checked option is not the first one, should that checked option receive focus when the overlay is opened with the keyboard, rather than the first option?

Issues:

  1. Multiple-select on popover: I can only Tab to the first two options.
  2. Multiple-select on popover: when I Tab into the second option and press Space to select it, it selects the first option instead.
  3. Action sheet: when I Tab to open it, I do not see the focus indicator on the wrapper the way I do with the alert. This happens both with no value and with a value selected.

Comments:

  1. The action sheet and alert behave differently from each other, and I find the wrapper ring surprising in general. I would have expected the focus indicator to be on the selected option when an overlay opens with a value, not on the wrapper. You mentioned the wrapper ring is the intended behavior: is that intended based on web standards, or is it inherited from the existing overlay focus-trap?

Comment thread core/src/components/item/item.tsx
Comment thread core/src/components/modal/test/sheet/modal.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated
Comment thread core/src/components/select/test/basic/select.e2e.ts Outdated

@thetaPC thetaPC left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants